Skip to content

Conversation

TimWolla
Copy link
Member

See individual commits.

…lers()`

There is no one time fits all solution to initialization of the object
handlers. A follow-up commit will use distinct `create_object` handlers for
each parser class.

Explicitly spelling out the handlers is a well-established pattern in php-src
and I don't see a reason to diverge from that with an intransparent helper
method.
@TimWolla TimWolla force-pushed the uri-object-handlers branch from 239b203 to 47b8e57 Compare September 6, 2025 21:34
@TimWolla TimWolla marked this pull request as ready for review September 6, 2025 21:39
@TimWolla TimWolla requested review from nielsdos, kocsismate and a team September 6, 2025 21:39
@TimWolla
Copy link
Member Author

TimWolla commented Sep 6, 2025

For the RMs: This is halfway between feature and bugfix. It doesn't fix an actual bug, but will make the URI objects much more robust internally, since they are created in a “safe state”.

Copy link
Member

@DanielEScherzer DanielEScherzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RM approval, minimal technical review performed

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly fine, one nit where I think at least the commit message should be updated to not be misleading

…pon creation

This makes the objects much safer to use, since the `.parser` will always be
available and matching the object.
The parser for a given object is already known from the object itself and
particularly must never change. Reassigning the value in `uri_unserialize()` is
just unsafe, especially since the existing `->uri` is freed with the destructor
of the reassigned parser.

Just rely on the `->parser` field being set to the correct value.
Similarly to the previous change to `uri_unserialize()`, the `->parser` must
always match the object for the freeing to be safe.

Given that we expect to successfully parse URIs, we can eagerly initialize the
resulting URI object when using the `::parse()` methods and destruct it again
when parsing fails and `null` is returned instead. Calling the destructor is
safe, since `uri` will be `NULL`, which will result in a noop.

The `base_url_object` must also match the object that is currently being
constructed. Verify this using assertions matching the `->ce` and the
`->parser`.
@TimWolla TimWolla requested a review from nielsdos September 7, 2025 11:05
@TimWolla TimWolla force-pushed the uri-object-handlers branch from 47b8e57 to 89c1327 Compare September 7, 2025 11:05
@TimWolla TimWolla merged commit 4432083 into php:master Sep 8, 2025
9 checks passed
@TimWolla TimWolla deleted the uri-object-handlers branch September 8, 2025 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants